Exposing ServerConfig options #959
Conversation
|
Thanks for this! I'm glad to finally resolve that I would eliminate the first breaking change by having Could you also update CHANGELOG.adoc, adding a new "Breaking Changes" section under "Unreleased changes" that points to this issue and explains the extra default values? |
|
Thanks for the review! I updated it back to taking a config by reference and just cloned before passing to the inner server. (Now a much simple PR) Breaking change note has been added. Let me know if the tone of it makes sense. |
| @@ -49,6 +50,10 @@ pub struct ConfigDropshot { | |||
| pub bind_address: SocketAddr, | |||
| /// maximum allowed size of a request body, defaults to 1024 | |||
| pub request_body_max_bytes: usize, | |||
| /// maximum size of any page of results | |||
There was a problem hiding this comment.
do we want to add #[non_exhaustive] to the struct? Would that force consumers to use ..Default::default() when constructing this? i.e. such that adding more fields would not be a breaking change?
There was a problem hiding this comment.
I wondered the same thing. Consumers can already do that if they want. It's kind of nice that the way it is now you can have your code break if we add some new config option so that you get a chance to look yourself at whether our default works for you. I think I've used this in the past.
There was a problem hiding this comment.
We don't have to fix this here but I think I disagree with past-me here. If you want to do that, just check the changelog. We shouldn't need to bump a major to add new things.
A builder interface would be nice, too.
(I don't propose that we do anything in this PR.)
|
Sorry for letting this languish. Picking it back up now. I've got omicron mostly building (just a few dependency conflicts to resolve), and then I'll get a test run done. |
|
Omicron test suite passed with one test that failed due to exceeding a 60 second limit when running all of them. That test succeeded when run individually, so maybe that is my machine not providing enough resources to it. |
Do you have more information about the test failure? It does seem unlikely to be related but I like to be pretty sure. And if it's a flake, we should file an Omicron ticket. |
There was a problem hiding this comment.
Also @augustuswm do you have a link to the changes you had to make to Omicron to build with this? Thanks!
| @@ -49,6 +50,10 @@ pub struct ConfigDropshot { | |||
| pub bind_address: SocketAddr, | |||
| /// maximum allowed size of a request body, defaults to 1024 | |||
| pub request_body_max_bytes: usize, | |||
| /// maximum size of any page of results | |||
There was a problem hiding this comment.
We don't have to fix this here but I think I disagree with past-me here. If you want to do that, just check the changelog. We shouldn't need to bump a major to add new things.
A builder interface would be nice, too.
(I don't propose that we do anything in this PR.)
|
This is a bit behind main now at this point but here is a commit (with patched deps targeting my local versions): oxidecomputer/omicron@a646a8f Two notable bits:
I'll try some test runs here again to try and pick up the actual test that failed, I should have noted it down before. |
|
Cool. Those diffs look good. We've gone back and forth on putting the |
I wanted to start a discussion on how we might go about exposing some additional server settings to the end user, namely the pagination default values. This PR is the most straightforward take and does not consider backwards compatibility. The main changes are:
ServerConfigwas usedConfigDropshotis now usedConfigDropshotduring construction, the value is passed.ConfigDropshot.page_max_nitemsandpage_default_nitems.This causes two breaking changes:
Defaultimplementation will have to add the new fields.The are a couple options we could also consider here.
ConfigDropshotalready derives Clone. We could continue accepting a reference and clone the users config. Not as ideal, but one less breaking change.ConfigDropshotstruct, we could consider a builder for it. I'm not sure how helpful this is at the moment given theDefaultimpl.Either way the primary goal here is to come up with a pattern to expose some of these configurations to end users at the server level (specifically ignoring the ability to control these settings per endpoint).